Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added realignment to dlgp and pdm #428

Open
wants to merge 14 commits into
base: release-1.0
Choose a base branch
from

Conversation

JonathanAellen
Copy link
Contributor

@JonathanAellen JonathanAellen commented Apr 29, 2024

introduced

added functionality to realign a model, simulating a different pose of the original training data.

the function '.realign(ids)' can be accessed from DiscreteLowRankGaussian and PointDistributionModel.

@JonathanAellen JonathanAellen marked this pull request as ready for review June 10, 2024 12:45
Copy link
Contributor

@marcelluethi marcelluethi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the PR. This is a very valuable addition.
I added a few minor comments, but find that it already looks good.

@@ -485,6 +505,12 @@ object DiscreteLowRankGaussianProcess {
new DiscreteLowRankGaussianProcess[D, DDomain, Value](domain, meanVec, varianceVec, basisMat)
}

def unapply[D: NDSpace, DDomain[D] <: DiscreteDomain[D], Value](
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add an unapply method, we should also remove the private modifier from the corresponding fields, and maybe add an apply method. Like this it is a bit contradictory.
It was private so far to prevent access to it, but I don't have a strong opinion to keep it like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i forgot to remove the unapply method. will be removed in the next version

* types the whole discrete low rank gp to make sure that it is applied to the appropriate models. The value type could
* be left out if the user knows what to do.
*/
trait RealignExtendedBasis[D: NDSpace, Value]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the NDSpace context bound here? If not, it should be omitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right. good catch

@marcelluethi
Copy link
Contributor

It would be great if @Andreas-Forster or somebody else from Shapemeans could have a look at it regarding the user interface and practical usability.

Copy link
Member

@Andreas-Forster Andreas-Forster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you a lot for this addition. I believe this is something very useful.

I gave it a very short try with a random model lying around and my findings are the following:

  • It feels natural to call the realign method from a PDM.
  • The specified poins were stable when sampling.
  • But, some of my expectations are not fulfilled. Mainly, I would have expected, that the same coefficients could produce more or less the same model instance. This seems not to be the case. The documentation does not give me a hint, nor where to look for a documentation of the implemented method.
  • Could we also have a translation-only realign method? Or would that be meaningless?

I would love to see the documentation refer to a more in-depth explanation of the used algorithm before merging.

@JonathanAellen
Copy link
Contributor Author

Thanks for taking a look:

  • The diagonalize setting can be turned off such that the same (or very similar with withExtendedBasis = true) coefficients create the same shapes in the different models. The disadvantage is that the model then relies on a non-diagonalized basis, which some other provided functions don't expect. Calls to instance or sample are correct but marginalLikelihood provides the wrong result i think. i'll make that clearer in the documentation. I could also add an alternative function call that returns a matrix that could be used to move samples between the different spaces. Then i would see the argument to always diagonalize to avoid a problematic basis.
  • to align only over translation withExtendedBasis should be set to false. Then there is also a one-to-one relationship between the model spaces. I'll make the documentation clearer.

I am not sure there is a suitable in-depth explanation of the used algorithm at the moment.

@marcelluethi
Copy link
Contributor

Thanks, @Andreas-Forster for testing it.
As we have correspondence, we could just recover the corresponding coefficients using the project method. To me it does not feel natural to return a projection matrix or keep the basis non-orthonormal.

What do you think?

@Andreas-Forster
Copy link
Member

I agree, keeping the basis non-orthonormal might confuse even more. And the projection matrix is not needed, as you mentioned, we have a project method.

I think updating the documentation and maybe later, if available link some more verbose document might be valuable.

…sis. slight refactor to RealignExtendedBasis
@JonathanAellen
Copy link
Contributor Author

I did some further testing about the influence of a non orthonormal basis in scalismo. It seems the DiscreteLowRankGaussian can handle such a non diagonalized basis. There are also various other operations already in the class that modify an orthonormal basis and lead to a non orthonormal shape model.

The only thing that i regularly do that shouldn't be done with a non orthonormal basis is directly accessing the variance field to check the overall variance of the model. without considering the basis this is incorrect for a non orthonormal model. All implementations i looked at are written to handle non orthonormal basis.

So i can see that this comes down to preference. Closer equivalence between model spaces versus a more intuitive formulation of the resulting model space.

Copy link
Member

@Andreas-Forster Andreas-Forster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding this to the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants